[pull] main from react:main - #595
Merged
Merged
Conversation
`formatConsoleArgumentsToSingleString` in
`packages/react-devtools-shared/src/backend/utils/index.js`
inlines `console.*` printf-style substitutions into a single string.
That string is
used both as the dedup key and as the displayed text for per-component
warnings/errors.
The `switch` that consumes the captured flag handles `s`, `d`, `i`, and
`f`, and the
function's own header comment says it "Implements s, d, i and f
placeholders". But
the substitution regex only captured `[jds]`:
```js
const REGEXP = /(%?)(%([jds]))/g;
```
So `%i` and `%f` were never matched. The `case 'i'` and `case 'f'` arms
were dead
code: the specifier was emitted literally and its argument was never
consumed. Worse,
because the unmatched specifier does not shift its argument, every
following specifier
in the same format string then binds to the wrong argument (a cascading
off-by-one
over the remaining args).
`%i` and `%f` are standard console integer/float specifiers (Node
`util.format` and
browsers both support them), so this affected common log formats. The
fix adds `i`
and `f` to the regex class so the existing switch arms run:
```js
const REGEXP = /(%?)(%([jdisf]))/g;
```
This is a one-character-class change that reconciles the regex with the
switch and
the header comment. The pre-existing behavior that `%j` is matched but
has no
`case` (so it falls through unchanged) is intentionally left as-is; it
is out of
scope for this fix.
## How did you test this change?
Added three regression tests to the existing
`formatConsoleArgumentsToSingleString`
describe block in
`packages/react-devtools-shared/src/__tests__/utils-test.js`:
- `formatConsoleArgumentsToSingleString('%i', 3.14)` -> `'3'`
- `formatConsoleArgumentsToSingleString('%f', 3.5)` -> `'3.5'`
- `formatConsoleArgumentsToSingleString('a %i b %s', 7, 'x')` -> `'a 7 b
x'` (locks
in argument alignment)
Commands run locally (experimental devtools bundles):
```
yarn build-for-devtools
yarn test --build --project=devtools -r=experimental packages/react-devtools-shared/src/__tests__/utils-test.js
```
Result: `Tests: 53 passed, 53 total`.
To confirm the tests actually cover the bug, I reverted the
one-character fix back to
`[jds]` and reran: the three new tests fail exactly as the bug predicts,
e.g. `%i`
yields `"%i 3.14"` and `a %i b %s` yields `"a %i b 7 x"` (the `%s` binds
to `7`
instead of `x`, showing the off-by-one). Restoring the fix makes them
pass again.
Also green:
```
yarn linc # ESLint on changed files: passed
yarn prettier # no files reflagged
yarn flow dom-node # No errors!
```
…supplied (#36930) `formatConsoleArguments` in `packages/react-devtools-shared/src/backend/utils/formatConsoleArguments.js` is used by the DevTools backend (via `hook.js`) to inline `console.*` printf-style substitutions after stripping React's appended component stack. For `%s`/`%d`/`%i`/`%f` it consumes the next argument with `args.splice(argumentsPointer, 1)` and formats the result. When a format string has more specifiers than arguments, `splice` returns an empty array, so `arg` is `undefined` and the specifier is rendered as text: `%s` becomes `"undefined"`, and `%d`/`%i`/`%f` become `"NaN"`. ```js formatConsoleArguments('%s %s', 'the'); // before: ['the undefined'] // after: ['the %s'] ``` Browsers and Node's `util.format` leave an unmatched specifier as a literal (`console.log('%s %s', 'a')` prints `a %s`; `console.log('%d')` prints `%d`). So a message like `console.warn('value: %s')` was shown in DevTools as `value: undefined` instead of `value: %s`. This guards each of the `%d`/`%i`, `%f`, and `%s` cases on argument availability (`argumentsPointer >= args.length`): when nothing is left to consume it keeps the specifier text and does not splice, mirroring the existing trailing-`%` handling added in #36852. An explicitly passed `undefined`/`null` argument is unchanged and still renders as `undefined`/`null`, since a value is present at that position (the `formats nullish values` test still passes). ## How did you test this change? Added a regression test to the existing `formatConsoleArguments` describe block in `packages/react-devtools-shared/src/__tests__/utils-test.js`: ```js it('keeps specifiers literal when no argument is supplied', () => { expect(formatConsoleArguments('%s %s', 'the')).toEqual(['the %s']); expect(formatConsoleArguments('%s %d', 'value')).toEqual(['value %d']); expect(formatConsoleArguments('%s %i', 'value')).toEqual(['value %i']); expect(formatConsoleArguments('%s %f', 'value')).toEqual(['value %f']); }); ``` Each assertion fails on `main` (it produces `['the undefined']` and `['value NaN']`) and passes with the fix. Commands run locally: ``` yarn test --build --project devtools packages/react-devtools-shared/src/__tests__/utils-test.js # 51 passed, 51 total yarn lint packages/react-devtools-shared/src/backend/utils/formatConsoleArguments.js \ packages/react-devtools-shared/src/__tests__/utils-test.js # Lint passed. yarn flow dom-node # No errors! yarn prettier-check packages/react-devtools-shared/src/backend/utils/formatConsoleArguments.js \ packages/react-devtools-shared/src/__tests__/utils-test.js # clean ``` Cross-checked the expected output against Node `util.format`: `util.format('%s %s', 'the')` -> `the %s`; `util.format('%d')` -> `%d`.
) `validateChildKeys` in `packages/react/src/jsx/ReactJSXElement.js` had a signature of `validateChildKeys(node, parentType)`, but #34174 ("Remove unused arguments from ReactElement") dropped the second argument, changing the signature to `validateChildKeys(node)` and updating every call site to pass a single argument. That change removed several now-unused `@param` lines from the same file, but left one behind on `validateChildKeys`: ```js /** * ... * @internal * @PARAM {ReactNode} node Statically passed child of any type. * @PARAM {*} parentType node's parent's type. // <- no such parameter anymore */ function validateChildKeys(node) { ``` `parentType` no longer appears anywhere in the function signature or body, so this `@param` line is stale and misleading to anyone reading the doc comment. This PR deletes that single line. The remaining `@param {ReactNode} node` already fully and correctly documents the sole parameter. No code or behavior change. ## How did you test this change? This is a documentation-only change (a JSDoc comment on an `@internal` helper), so there is no runtime behavior to test. I verified it as follows: - Confirmed `parentType` no longer appears anywhere in `packages/react/src/jsx/ReactJSXElement.js` (`grep -n parentType` returns no matches after the change). - Confirmed the signature `function validateChildKeys(node)` and all call sites are unchanged by this diff. - `yarn prettier` (via `scripts/prettier/index.js check-changed`) - clean. - `yarn linc` (ESLint on changed files) - passed. - `yarn flow dom-node` - No errors. --- ## Diff (for reference) ```diff diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js @@ -860,7 +860,6 @@ export function cloneElement(element, config, children) { * * @internal * @PARAM {ReactNode} node Statically passed child of any type. - * @PARAM {*} parentType node's parent's type. */ function validateChildKeys(node) { ```
…change (#36935) `printOperationsArray` in `react-devtools-shared` walks an operations array under the invariant that each `switch` case leaves `i` pointing at the next opcode (loop header at `packages/react-devtools-shared/src/utils.js`). The `TREE_OPERATION_APPLIED_ACTIVITY_SLICE_CHANGE` case broke that invariant: ```js case TREE_OPERATION_APPLIED_ACTIVITY_SLICE_CHANGE: { i++; // skip opcode -> i now at the value slot const activitySliceIDChange = operations[i + 1]; // reads the slot AFTER the value; i not advanced ... } ``` The operation is exactly two slots, `[opcode, activitySliceID]` (see the writer in `packages/react-devtools-shared/src/backend/fiber/renderer.js`, which pushes the opcode then the id). So the case did two things wrong: 1. It logged the wrong number: `operations[i + 1]` reads the slot *after* the value (the next operation's opcode, or `undefined` at the end of the array). 2. It left `i` pointing at the value slot, so the outer `while (i < operations.length)` loop re-read the activity-slice id as an opcode. For any non-zero slice id that falls through to `default: throw Error("Unsupported Bridge operation ...")`, aborting the whole dump. The two canonical decoders of this same operation both use the correct pattern (skip the opcode, then read *and* advance past the value): - `devtools/store.js`: `i++; nextActivitySliceID = operations[i++];` - `devtools/views/Profiler/CommitTreeBuilder.js`: `i++; const activitySliceIDChange = operations[i++];` This change makes `printOperationsArray` match them by reading `operations[i++]`. This is a debug-only diagnostic path: the only caller is the `__DEBUG__`-guarded dump in `backend/legacy/renderer.js`, so it is not a production crash. The bug was introduced in #34908. ## How did you test this change? Added a regression test for `printOperationsArray` in `packages/react-devtools-shared/src/__tests__/utils-test.js`. The fixture chains two activity-slice operations, `[rendererID, rootID, stringTableSize=0, opcode, 42, opcode, 0]`; the trailing operation is what forces the reader to advance past the first value slot rather than re-read it. It asserts the call does not throw, logs once, and that the message contains both `Applied activity slice change to 42` and `Reset applied activity slice`. Ran the DevTools Jest project (built first, as that project requires a build): - With the fix: 51/51 pass, including the new test. - Reverting only the one-line fix back to `operations[i + 1]` and rebuilding: the new test fails with `Unsupported Bridge operation "42"` (exactly the predicted failure), 50 pass / 1 fail. Restored the fix and it is green again. `yarn prettier-check` and `yarn linc` are clean on the changed files.
## Summary In a large react app, especially when components having similar starting names like Table, TableColumn, TableCell, TableRow all together 100+ components when rendered in a virtualized table. Traversing the search result is sometimes difficult with scroll The component tree search only let you step through matches one at a time (Enter / Shift+Enter). In large apps with many similarly-named components (Table, TableRow, TableCell, ...) a search can return 100+ matches in a virtualized list, making a specific match tedious to reach. - the result counter is an editable, live-scrubbing index field: typing a number scrolls to that match as you type (clamped to range) - Fixes re-search getting stuck, clearing the box and retyping the same term while a match was still selected snapped back to that same component. It now advances to the next match (find-next semantics). ## How did you test this change? Adds a SearchableTable example to the DevTools shell and unit tests for the new action and the retype behavior. https://github.com/user-attachments/assets/7ea9801a-7bcb-4e8f-bf73-a5307a0fdbae cc @hoxyq Let me know what do you feel about this feature, if its helpful for devtools.
Quick follow-up to #36786, which wasn't rebased onto version of `main` that already had Flow upgraded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )